Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix product publications #2774

Merged
merged 15 commits into from
Sep 7, 2017
Merged

Fix product publications #2774

merged 15 commits into from
Sep 7, 2017

Conversation

spencern
Copy link
Contributor

Fixes hacky product publications that got added to marketplace in haste a couple days ago.

Specifically:
Security Fix

  • Fixes bug where users with permissions for any shop, were considered to have that permission for all shops by Reaction.hasPermission. This was code that was leftover from the original sellerShops implementation of marketplace.

New Methods

  • Adds Reaction.getShopsWithRoles method which provides a list of shopIds that the user has at least one of the provided roles for.

Updates

  • the Products publication now publishes admin products for all shops that a user has access for.
  • The productMedia publication helper now publishes media for all published products, regardless of the shopId
  • The Product publication was updated to check if a user has roles for the specific product that is being published, not the active shop. This fixes an issue where if a user had their shop active and browsed another's products, they could see them as if they were an admin.

@spencern spencern requested a review from mikemurray August 31, 2017 02:21
@spencern
Fixes tests by adding additional stubs for hasPermission and getShops…
04452f0 
…WithRoles

Massively simplify Product publication.
Pass `this.userId` from publications into getShopsWithRoles to satisfy tests
Adds stubs for Reaction.hasPermission and getShopsWithRoles
@spencern
Merge branch 'marketplace' into spencer-fix-product-publications
601320d
@spencern
touch some docs to get bithound to update
89895e4
@spencern
Add menuClassName propType to fix bitHound error
b57c7cd
@spencern
Copy link
Contributor Author

spencern commented Sep 5, 2017

@mikemurray can you give this a review sometime soon?

@mikemurray
Copy link
Member

Yeah, reviewing now.

Question. I have 2 brand shops and the primary shop signed into three browsers. I on the homepage, while signed in as brand 2, I cannot see the products from brand 1.

Should I only be able to see my products? Or, is this related to #2760.

reaction_and_reaction_and_reaction_and_fix_product_publications_by_spencern_ pull_request__2774 _reactioncommerce_reaction

@spencern
Copy link
Contributor Author

spencern commented Sep 6, 2017 •
edited
Loading

@mikemurray
Are those products published? You should not be able to see hidden products from other brands. I'm also thinking we should integrate with the "edit mode" switch and only show products you have admin access for when you're in edit mode.

I thought this was publishing all public products as well as products you have admin access for, but it may be that the admin publication supersedes the other publication and results in the other products never getting sent.

Now that I've thought about it a little more, it may be good to not publish products that a user doesn't have admin access for when in "edit mode." Having a mixture of editable and non-editable products may cause chaos for the UI and the bulk publish/edit actions.

@mikemurray
Copy link
Member

@spencern It seems that the admin publication is superseding the "normal user" part of that publication. As an anonymous user, I can see the published products from all shops. But as a brand, I can only see my own shops products on the main grid.

@spencern
Copy link
Contributor Author

spencern commented Sep 6, 2017

What do you think about having the publication switch with the "edit mode" switch? Is that too confusing? Any thoughts for how we should we publish products that a user doesn't have admin access for when a user is an admin for at least one shop? (does that make sense?)

@mikemurray
Copy link
Member

The edit mode might be helpful, at least from the standpoint of being clear that these are my products to edit; you can get that from Meteor.user().profile on the server, or pass it in. Probably don't need to edit mode boolean, since the publication should know for which shop its publishing products for, and if I'm an admin of that shop. And edit mode on client mostly just hides the edit buttons, badges, etc.

On the main product grid, we could just adjust the publication to only show products that a normal user would see. Then on the shop's own product grid homepage, we can, of course, show all the editable products for that shop.

So basically...

As shop owner, In edit mode

  • Homepage - sees grid like normal user
  • My Shops page - sees grid as admin
  • Other Shops page - sees grid like normal user

As shop owner in, NOT in edit mode ( viewing as a customer )

  • Homepage - sees grid like normal user
  • My Shops page - sees grid like normal user
  • Other Shops page - sees grid like normal user

@spencern
Copy link
Contributor Author

spencern commented Sep 6, 2017

I like this idea. Do we have a navigation link to "my shop" currently?

@mikemurray
Copy link
Member

We do not.

@spencern
Copy link
Contributor Author

spencern commented Sep 6, 2017 •
edited
Loading

Does the my-shops route exist? Are you referring to /:shop-slug/products?

@mikemurray
Copy link
Member

mikemurray commented Sep 6, 2017 •
edited
Loading

They may exist if you physically type into the address bar, or do a location.href to "/shopPrefix/thing" but I don't think it'll give you a URL param to use to filter by the shop. That's what may be solved with #2760

@spencern
Copy link
Contributor Author

spencern commented Sep 7, 2017

@mikemurray
I've updated this PR to show all products when an administrator has edit mode turned off. I think updating it to be route aware for a route that doesn't exist yet is beyond the scope of this PR and should perhaps be reconsidered after #2760 is finished.

As shop owner, In edit mode
Any Product Grid page: See only products they have edit permissions for (owner/admin/createProduct)

As shop owner in, NOT in edit mode ( viewing as a customer )
See all products as a customer would. They will not see their own products that are hidden.
I think this is probably preferable anyway as we've needed a way for admins to see what a given product grid looks like from a customer perspective.

Might be worth considering changing the language from "edit mode" to customer --* admin or something similar, but I think that would require additional Publication updates.

From my perspective this PR is ready to be fully reviewed and pulled in.

@mikemurray
Copy link
Member

@spencern checking now

mikemurray
mikemurray approved these changes Sep 7, 2017
View reviewed changes
Copy link
Member

@mikemurray mikemurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with the changes. We will have to revisit anyway with changes to the router and shop filtering.

@mikemurray mikemurray merged commit 76062b8 into marketplace Sep 7, 2017
@mikemurray mikemurray deleted the spencer-fix-product-publications branch September 7, 2017 21:37
@spencern spencern mentioned this pull request Oct 11, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikemurray mikemurray mikemurray approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@spencern @mikemurray